Skip to content

Conversation

@d10c
Copy link
Contributor

@d10c d10c commented Nov 28, 2025

Updates the (still internal, therefore no change note) MRVA Autofix feature to use the latest Go-based binary.

Can be configured to use either an env var (env:<ENV_VAR_NAME>) or a 1Password secret reference (op://<vault>/<item>/<field>; requires op command) for the Copilot API key. By default, it uses the CAPI_DEV_KEY env var, as before.

  • In the .md output, I'm seeing unescaped terminal escape codes like �[31m- }.run();�[0m. Was it like that before?
  • I'll invite discussion as to whether calling out to 1Password CLI is the best option, or if passing a path to a file containing the secret would be preferable.
Example autofix output

mysql/mysql-connector-j

Fix suggestion details
Changes:

Will patch: src/test/java/testsuite/regression/StatementRegressionTest.java
...
                  }
  
�[31m-             }.run();�[0m
�[32m+             }.start();�[0m
  

Description:

To fix this issue, replace the direct call to `run()` with a call to `start()`. The `start()` method initializes a new thread and then calls the `run()` method internally, ensuring that the code in `run()` executes asynchronously on a separate thread. This approach preserves the intended concurrency.

Steps:
1. Locate the erroneous line (`.run()`).
2. Replace `.run()` with `.start()` to ensure the `Thread` runs in its own thread.
3. Validate that this change aligns with the intended behavior of the code.

---


Fix is valid.

Notes

  • notes placeholder

chinabugotech/hutool

Fix suggestion details
Changes:

Will patch: hutool-core/src/test/java/cn/hutool/core/io/WatchMonitorTest.java
...
  	public void testDir() {
  		monitor = WatchMonitor.createAll("d:/", new DelayWatcher(watcher, 500));
�[31m- 		monitor.run();�[0m
�[32m+ 		monitor.start();�[0m
  	}

Description:

To fix the problem, we should properly start the `WatchMonitor` instance on a new thread instead of calling its `run()` method directly. This typically involves using the `start()` method, which ensures the `run()` method executes on a separate thread as intended.

In this case, the following changes are needed:
1. Replace `monitor.run()` with `monitor.start()` in the affected test methods: `testDir` and `testDelay`.
2. No other modifications appear necessary since the `monitor` object is already correctly set up.

---


Fix is valid.
Changes:

Will patch: hutool-core/src/test/java/cn/hutool/core/io/WatchMonitorTest.java
...
  	public void testDir() {
  		monitor = WatchMonitor.createAll("d:/", new DelayWatcher(watcher, 500));
�[31m- 		monitor.run();�[0m
�[32m+ 		new Thread(monitor).start();�[0m
  	}

Description:

To fix this issue, we need to ensure that the `run()` method is called on a separate thread instead of the current thread. If `WatchMonitor` is a `Thread` or similar, then `start()` should be invoked instead of `run()`. If `WatchMonitor` is not directly a thread but implements `Runnable`, a `Thread` instance wrapping the `WatchMonitor` object should be created and started. This approach preserves the expected behavior of starting the monitoring task in a separate thread.

There are two locations in the code where this issue appears: line 68 (`monitor.run()`) and line 84 (`monitor.run()`). Both should be modified to ensure a new thread is started.

---


Fix is valid.

Notes

  • notes placeholder

@d10c d10c force-pushed the d10c/mrva-go-autofix branch from a11ebc0 to c5972fe Compare December 1, 2025 11:47
@d10c d10c marked this pull request as ready for review December 1, 2025 12:10
@d10c d10c requested review from a team as code owners December 1, 2025 12:10
Copilot AI review requested due to automatic review settings December 1, 2025 12:10
Copilot finished reviewing on behalf of d10c December 1, 2025 12:11
@d10c d10c requested a review from jcogs33 December 1, 2025 12:12
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR migrates the internal MRVA Autofix feature from using a JavaScript-based Cocofix binary to a Go-based autofix binary. It also introduces flexible API key configuration that supports environment variables and 1Password secret references.

Key changes:

  • Added new configuration setting codeQL.autofix.capiDevKey for API key management
  • Implemented secure API key resolution with support for environment variables and 1Password CLI
  • Updated binary name from cocofix.js to autofix and adjusted related paths

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
extensions/ql-vscode/src/config.ts Adds new capiDevKey setting and getter function for autofix configuration
extensions/ql-vscode/src/variant-analysis/view-autofixes.ts Implements API key resolution logic, adds 1Password CLI integration, updates binary paths and names from Cocofix to Go autofix

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +181 to +182
}
if (capiDevKey.startsWith("op://")) {
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic allows a value that starts with env: but resolves to an empty string to fall through to the 1Password check. If the environment variable doesn't exist (line 180), capiDevKey becomes an empty string, but then line 182 checks if it starts with op://, which will be false. The empty string should trigger the error at line 185-188, but the control flow is confusing. Consider using else if at line 182 to make the mutually exclusive nature of these options clearer and prevent potential confusion.

Suggested change
}
if (capiDevKey.startsWith("op://")) {
} else if (capiDevKey.startsWith("op://")) {

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to allow the option of the environment variable containing an op:// reference.

Comment on lines +867 to +871
async function opRead(secretReference: string): Promise<string> {
return new Promise((resolve, reject) => {
const opProcess = spawn("op", ["read", secretReference], {
stdio: ["ignore", "pipe", "pipe"],
});
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the op command is not found on the PATH, the error from line 884-885 will be a system error like "ENOENT" which may not be clear to users. Consider catching this specific case and providing a more helpful error message indicating that the 1Password CLI needs to be installed and available on PATH.

Copilot uses AI. Check for mistakes.
}
if (!capiDevKey) {
throw new Error(
`Copilot API dev key not found. Make sure ${AUTOFIX_CAPI_DEV_KEY.qualifiedName} is set correctly.`,
Copy link

Copilot AI Dec 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This error message doesn't provide enough context about why the key wasn't found. Since the key could be missing due to an undefined environment variable or failed 1Password read, consider including which method was attempted (env var name or op:// reference) to help users debug the issue.

Suggested change
`Copilot API dev key not found. Make sure ${AUTOFIX_CAPI_DEV_KEY.qualifiedName} is set correctly.`,
`Copilot API dev key not found. Attempted to retrieve using '${getAutofixCapiDevKey() || "env:CAPI_DEV_KEY"}'. Make sure ${AUTOFIX_CAPI_DEV_KEY.qualifiedName} is set correctly and that the referenced environment variable or 1Password secret exists and is accessible.`,

Copilot uses AI. Check for mistakes.
@d10c d10c requested a review from nickrolfe December 1, 2025 12:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant